Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Encapsulate IA2 relations constants #13096

Merged
merged 3 commits into from Nov 30, 2021
Merged

Encapsulate IA2 relations constants #13096

merged 3 commits into from Nov 30, 2021

Conversation

feerrenrut
Copy link
Contributor

Link to issue number:

None

Summary of the issue:

IAccessible2 code that depends on fetching objects related to other objects uses these constants.
Since these values are strings, and not defined in an IDL file, we should encapsulate them in an
Enum.

Description of how this pull request fixes the issue:

Encapsulate the constants in a Relations enum under IAccessibleHandler.

  • Removed IA2_RELATION_FLOWS_FROM
  • Removed IA2_RELATION_FLOWS_TO
  • Removed IA2_RELATION_CONTAINING_DOCUMENT
  • Replaced with IAccessibleHandler.RelationType enum

This is a compatibility breaking change, however it's not expected that many Addons use this code.

All internal usages of the constants have been updated:

  • Double checked with git grep -i IA2_RELATION_
  • The only remaining results are in C++ files.

Testing strategy:

  • Run locally
  • System tests
  • Usage on alpha prior to release of NVDA 2022.1

Known issues with pull request:

Compatibility breaking change.

Change log entries:

For Developers
- IAccessibleHandler IA2_RELATION_* constants have been replaced with the IAccessibleHandler.RelationType enum.

Code Review Checklist:

  • Pull Request description:
    • description is up to date
    • change log entries
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • API is compatible with existing add-ons.
  • Documentation:
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English

@feerrenrut
Relations constants
658c374 
Compat breaking refactor:
- Removed IA2_RELATION_FLOWS_FROM
- Removed IA2_RELATION_FLOWS_TO
- Removed IA2_RELATION_CONTAINING_DOCUMENT
- Replaced with IAccessibleHandler.RelationType enum
@feerrenrut feerrenrut marked this pull request as ready for review November 26, 2021 02:47
@feerrenrut feerrenrut requested a review from a team as a code owner November 26, 2021 02:47
@michaelDCurran
Copy link
Member

We currently define all the IA2 relation constants in NVDA ourself, as although they are defined in IAccessible2's accessibleRelation.idl, they are all separate constants, not referenced directly or indirectly by the IAccessible2 Library definition, thus never included in the type library, and therefore never exposed to NVDA via comtypes.
I suppose they really should have been wrapped in their own namespace block in the idl file, and then that namespace block should be mentioned by the library definition.
I'm just mentioning this here to clarify why we do define the relation constants in NVDA.

@feerrenrut feerrenrut mentioned this pull request Nov 30, 2021
Merged
6 tasks
@feerrenrut
Copy link
Contributor Author

Thanks @michaelDCurran that is helpful to know. I guess we'll progress with this PR as it is, and consider getting that IDL changed?

@feerrenrut feerrenrut added api-breaking-change deprecated/2022.1 Label used to track deprecations due for removal in the 2022.1 release and removed api-breaking-change labels Nov 30, 2021
@feerrenrut feerrenrut merged commit 83125f2 into master Nov 30, 2021
@feerrenrut feerrenrut deleted the splitRelationsConstants branch November 30, 2021 06:42
@nvaccessAuto nvaccessAuto added this to the 2022.1 milestone Nov 30, 2021
@OzancanKaratas OzancanKaratas mentioned this pull request Nov 30, 2021
Closed
@LeonarddeR
Copy link
Collaborator

This breaks in Thunderbid:

error executing event: focusEntered on <NVDAObjects.IAccessible.mozilla.Mozilla object at 0x07481250> with extra args of {}
Traceback (most recent call last):
  File "eventHandler.pyc", line 284, in executeEvent
  File "eventHandler.pyc", line 98, in __init__
  File "eventHandler.pyc", line 105, in next
  File "eventHandler.pyc", line 133, in gen
  File "NVDAObjects\__init__.pyc", line 394, in _get_treeInterceptor
  File "treeInterceptorHandler.pyc", line 25, in getTreeInterceptor
  File "virtualBuffers\gecko_ia2.pyc", line 262, in __contains__
  File "virtualBuffers\gecko_ia2.pyc", line 238, in _iterIdsToTryWithAccChild
  File "virtualBuffers\gecko_ia2.pyc", line 196, in _getEmbedderFrame
  File "comtypes\__init__.pyc", line 851, in __call__
  File "monkeyPatches\comtypesMonkeyPatches.pyc", line 32, in __call__
ctypes.ArgumentError: argument 1: <class 'TypeError'>: unicode string expected instead of RelationType instance

@feerrenrut feerrenrut mentioned this pull request Dec 1, 2021
Merged
5 tasks
@feerrenrut
Copy link
Contributor Author

@LeonarddeR a fix is in progress (see PR #13112). If possible, please test the PR build, thanks!

feerrenrut added a commit that referenced this pull request Dec 2, 2021
@feerrenrut
Fix Enum str mixup (PR #13112)
a46d8d6 
- The RelationType enum now also inherits from str, its values can now be used as a string value.
- Fixed conversion of plain str types to RelationType
- Use faster approach to get relations of type (using dedicated method in IAccessible_2_2)

Fixes #13109 - Thunderbird and Firefox were broken with PR #13096.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelDCurran michaelDCurran michaelDCurran approved these changes

Assignees
No one assigned
Labels
deprecated/2022.1 Label used to track deprecations due for removal in the 2022.1 release
Projects
None yet
Milestone
2022.1
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@feerrenrut @michaelDCurran @LeonarddeR @nvaccessAuto